Skip to content

Conversation

colleenmcginnis
Copy link
Contributor

Removes attribute includes in index*.asciidoc files that are subsections of the new Serverless book. These files will inherit attributes from serverless/index.asciidoc.

h/t @leemthompo for pointing out incorrect attribute values on rendered pages!

@colleenmcginnis colleenmcginnis self-assigned this Nov 7, 2024
Copy link

github-actions bot commented Nov 7, 2024

A documentation preview will be available soon.

Request a new doc build by commenting
  • Rebuild this PR: run docs-build
  • Rebuild this PR and all Elastic docs: run docs-build rebuild

run docs-build is much faster than run docs-build rebuild. A rebuild should only be needed in rare situations.

If your PR continues to fail for an unknown reason, the doc build pipeline may be broken. Elastic employees can check the pipeline status here.

@colleenmcginnis

This comment was marked as outdated.

@colleenmcginnis
Copy link
Contributor Author

For the reviewer: You can see the result of this change by going to this diff in the built-docs repo or looking at diff page and comparing the preview link to the production link:

attr-compare.mov

@colleenmcginnis colleenmcginnis marked this pull request as ready for review November 7, 2024 18:17
@colleenmcginnis colleenmcginnis requested a review from a team as a code owner November 7, 2024 18:18
Copy link
Contributor Author

@colleenmcginnis colleenmcginnis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the comment below would be a helpful reminder, we can add it to each index file.

@leemthompo
Copy link
Contributor

leemthompo commented Nov 7, 2024

@colleenmcginnis there'll be just a couple of smelly ones with this change, see this grep for an example. I wonder if we need to still have a bare attribute for simply "Elasticsearch"?

@leemthompo
Copy link
Contributor

But could also just be cleaned up in follow up by ES writers!

@colleenmcginnis
Copy link
Contributor Author

colleenmcginnis commented Nov 7, 2024

It's up to you and your team. If you want separate attributes for serverless and stateful, you could create two unique keys with unique values, add both to the shared attributes in the /docs repo, and update throughout the docs as you see fit.

Note: This is not unique to the move to AsciiDoc. There was only one value for the es variable in the docs-site repo, too, so it was probably rendering strangely in the Docsmobile docs, too. 🤷

@leemthompo
Copy link
Contributor

I need to look at this again tomorrow because too late for my feeble brain 😵‍💫. But I fear it might be a cognitive ouchy to have the same attribute do different things whether we're working in the serverless book or not, but yeah it's not a huge deal anyways. We had es-serverless but I don't think we were consistent one way or another.

@lcawl
Copy link
Contributor

lcawl commented Nov 7, 2024

With respect to the attributes that we're overloading in

// The values of these attributes are different in stateful vs serverless

  1. serverless-full and serverless-short is just a capitalization difference, so I think we should just synch on one and define it only in the docs repo
  2. Likewise I think we should find out which of the "Elasticsearch Serverless" or "Elasticsearch on serverless" is preferred and sync on that for es-serverless. If there's really a necessary difference here, IMO we'd add a new attribute for it in the docs repo.
  3. For the overloaded es attribute, of which there are 202 occurrences in the docs-content repo, I think we should review those occurrences and change them to es-serverless if appropriate then remove this override.
  4. For the overloaded observability and security attributes, I'd recommend creating obs-serverless and sec-serverless (or some such variation) in the shared attributes in the docs repo, then using them appropriately. There were 6 occurrences of {observability} and 0 occurrences of {security} in docs-content, but I didn't dig into the other repos.

If we really do need to keep overloading some of those terms, we could always create a shared/serverless/main.asciidoc file in the docs repo to define them there rather than at the book level. As serverless content is sprinkled beyond a single book. however, I think it's better to just use the specific, appropriate attribute in each case.

@colleenmcginnis
Copy link
Contributor Author

colleenmcginnis commented Nov 7, 2024

Do we want the ability to use the same source content for both stateful and serverless docs when the only difference between the content is the product names? If yes, I think it makes sense to have one attribute key that has two values depending on the context.

(An aside: If/When we consolidate serverless and stateful docs into a single page, which product name would we use?)

@lcawl
Copy link
Contributor

lcawl commented Nov 7, 2024

Do we want the ability to use the same source content for both stateful and serverless docs when the only difference between the content is the product names?

I would hope we don't have any pages where that's true, since if there aren't any differences other than the product names it's not worth having two pages IMO. Maybe this exists in the short-term book-based context but ideally not in the long term.

(An aside: If we were to consolidate serverless and stateful docs into a single page, which product name would we use?)

I think this is the kind of exercise we need to try with real examples, since IMO you could go for the most generic names or most specific names depending on the context.

For something very high-level and conceptual like https://www.elastic.co/guide/en/kibana/current/alerting-getting-started.html and https://www.elastic.co/guide/en/serverless/current/rules.html, there's no real difference in its behaviour across these contexts, so the most general possible references would work IMO.

For something lower-level like https://www.elastic.co/guide/en/cloud/current/ec-invite-users.html and https://www.elastic.co/guide/en/serverless/current/general-manage-access-to-organization.html, I think we'd use the most specific product/solution/deployment names to call out any differences (though in this case I think they're pretty much identical too).

@colleenmcginnis
Copy link
Contributor Author

colleenmcginnis commented Nov 7, 2024

Maybe this exists in the short-term book-based context but ideally not in the long term.

Yea, I guess I was thinking in the short term as we try to prepare for what's next, but are still waiting for v3 to be ready. But, if we don't want to consolidate any source files while we wait, I guess it's worth the effort to look at each instance of es, observability, and security to determine which should be changed to a *-serverless alternative. I'll leave it to each team to do that on their section of the docs.

Proposed next steps:

  • I'll open a PR in /docs to add es-serverless, obs-serverless, and sec-serverless attributes. (added to @lcawl's Add serverless shared attributes docs#3109)
  • @elastic/platform-docs: You can use this PR if you'd like to determine which es / observability / security instances should be changed to the *-serverless version throughout all docs-content content.
  • @elastic/obs-docs: I'll use Fix stateful vs serverless attribute conflicts observability-docs#4487 to determine which es / observability / security instances should be changed to the *-serverless version throughout all observability-docs content.
  • @elastic/security-docs: You can open a fresh PR to determine which es / observability / security instances should be changed to the *-serverless version throughout all security-docs content.

@lcawl
Copy link
Contributor

lcawl commented Nov 8, 2024

I've added a commit to remove the redundant variables after elastic/docs#3109 was merged.

@leemthompo
Copy link
Contributor

leemthompo commented Nov 8, 2024

Apologies if I haven't caught up fully here haven't been able to dig in but here are some thoughts

  1. serverless-full and serverless-short is just a capitalization difference, so I think we should just synch on one and define it only in the docs repo

This isn't a capitalization issues AFAICT it's Elastic Cloud Serverless versus Serverless.
I don't think we need to capitalize serverless when it's isolated so I think we can potentially remove both of these. In any case I think we only need Elastic Cloud Serverless.

Likewise I think we should find out which of the "Elasticsearch Serverless" or "Elasticsearch on serverless" is preferred and sync on that for es-serverless. If there's really a necessary difference here, IMO we'd add a new attribute for it in the docs repo.

I think it's "Elasticsearch Serverless" so we should stick to that one IMO.

For the overloaded es attribute, of which there are 202 occurrences in the docs-content repo, I think we should review those occurrences and change them to es-serverless if appropriate then remove this override.

There are few cases where we just need bare Elasticsearch so I'd vote for replacing all {es}s with {es-serverless}s and removing the override, and ES writers can fix up any stragglers over time. We're going to be doing a lot of cleanup there in the next week or so anyways.

@leemthompo
Copy link
Contributor

I'd vote for replacing all {es}s with {es-serverless}s and removing the override, and ES writers can fix up any stragglers over time

I can do this on Monday :)

@colleenmcginnis
Copy link
Contributor Author

Oh gosh this is getting so confusing 😵‍💫

serverless-full and serverless-short is just a capitalization difference, so I think we should just synch on one and define it only in the docs repo

This isn't a capitalization issues AFAICT it's Elastic Cloud Serverless versus Serverless.

I think Lisa was referring to:

In https://github.com/elastic/docs/pull/3109/files we updated the AsciiDoc values to match the Docsmobile values.

I don't think we need to capitalize serverless when it's isolated

It looks like you (@leemthompo) updated the serverless-short value to be capitalized a couple weeks ago in https://github.com/elastic/docs-site/pull/49. 😝

@leemthompo
Copy link
Contributor

leemthompo commented Nov 8, 2024

😵‍💫 Here's some more words to add to this salad:

  • I don't think we need a variable for the term serverless and I also don't think we ever need to capitalize Serverless on its own. So I don't think we need serverless-short ever.
  • I think "Elastic Cloud Serverless" is the correct capitalization, but I don't like the serverless-full variable because it's not immediately clear to an arbitrary contributor what this refers to. Maybe elastic-serverless is better.

@leemthompo
Copy link
Contributor

It looks like you updated the serverless-short value to be capitalized a couple weeks ago

That's water under the bridge. Those were simpler times.

Copy link
Contributor

@leemthompo leemthompo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can all scatter and revisit our use of attributes in our content sets separately?

@colleenmcginnis
Copy link
Contributor Author

Sure. If you don't want to make any other updates here, feel free to merge.

@leemthompo leemthompo merged commit e5d3b43 into elastic:main Nov 12, 2024
2 checks passed
@colleenmcginnis colleenmcginnis deleted the fix-attr branch November 12, 2024 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants